Ide 1.5.x Cleaned up a few warnings and fixed strict-aliasing #1399
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Cleaned up warnings about unsigned types, unused variables, and declarations. Explicitly declare char as signed for cases when compiling with -funsigned-char. Fixed strict-aliasing issues in IPAddress.h. Fixed array overrun where EXTERNAL_NUM_INTERRUPTS was defined too small.
Local variables can't be declared with PROGMEM and don't need to be when pointing to a PROGMEM address.
|
Looking at this patch:
|
|
Oh, and the PROGMEM thing was also fixed in a different way already. |
matthijskooijman
added a commit
to matthijskooijman/Arduino
that referenced
this pull request
Feb 19, 2014
Previously, pointer casting was used, but this resulted in strict-aliasing warnings:
IPAddress.h: In member function ‘IPAddress::operator uint32_t() const’:
IPAddress.h:46:61: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
operator uint32_t() const { return *((uint32_t*)_address); };
^
IPAddress.h: In member function ‘bool IPAddress::operator==(const IPAddress&) const’:
IPAddress.h:47:81: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
bool operator==(const IPAddress& addr) const { return (*((uint32_t*)_address)) == (*((uint32_t*)addr._address)); };
^
IPAddress.h:47:114: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
bool operator==(const IPAddress& addr) const { return (*((uint32_t*)_address)) == (*((uint32_t*)addr._address)); };
Converting between unrelated types like this is commonly done using a union,
which do not break the strict-aliasing rules. Using that union, inside
IPAddress there is now an attribute _address.bytes for the raw byte
arra, or _address.dword for the uint32_t version.
Since we now have easy access to the uint32_t version, this also removes
two memcpy invocations that can just become assignments.
This patch does not change the generated code in any way, the compiler
already optimized away the memcpy calls and the previous casts mean
exactly the same.
This is a different implementation of a part of arduino#1399 and it helps
toward fixing arduino#1728.
matthijskooijman
added a commit
to matthijskooijman/Arduino
that referenced
this pull request
Feb 19, 2014
peekNextDigit() returns an int, so it can return -1 in addition to all 256 possible bytes. By putting the result in a signe char, all bytes over 128 will be interpreted as "no bytes available". Furthermore, it seems that on SAM "char" is unsigned by default, causing the "if (c < 0)" line a bit further down to always be false. Using an int is more appropriate. A different fix for this issue was suggested in arduino#1399. This fix helps towards arduino#1728.
|
I'd need to review the change related to IPAddress, but the idea was that IPAddress would contain the address in network-byte order which is always big-endian regardless of architecture. Since I move the bytes via explicit offsets, it's unaffected by the compiler's native endianess (AKA host-byte order). The order that the bytes move over SPI is always going to be the same since that's defined by the WizNet chip and should be network byte order. The original code was sensitive to the compiler endianess. |
|
@penguin359 The original code, as well as my changes, store the IP address in the "obvious order", e.g. offsets are like 0.1.2.3. When converting to uint32_t, the compiler endianness indeed comes into play, which is little-endian on AVR, so the MSB would be the last part in the IP address, not the first part. Looking at your code, indeed it doesn't just fix the warning but also removes the endianness dependency. However, I think your code is the same as the original and my code on a little endian compiler, not a big endian one? I can actually imagine that the uint32_t conversion should always use big-endian interpretation, since then the first part of an IP ends up in the MSB, which is probably what people expect to happen. However, that's a separate change, that affects API compatibility, so that would need separate discussion on the developer's list. |
cmaglie
added
feature request
|
As stated above, most of the things in this PR are already fixed, others are detable. In its current form, this PR is no longer useful. If some of the warnings it fixes still occur and you want to fix them, creating a new PR would be the best approach. I'm closing this one. |
ollie1400
pushed a commit
to ollie1400/Arduino
that referenced
this pull request
May 2, 2022
Previously, pointer casting was used, but this resulted in strict-aliasing warnings:
IPAddress.h: In member function ‘IPAddress::operator uint32_t() const’:
IPAddress.h:46:61: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
operator uint32_t() const { return *((uint32_t*)_address); };
^
IPAddress.h: In member function ‘bool IPAddress::operator==(const IPAddress&) const’:
IPAddress.h:47:81: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
bool operator==(const IPAddress& addr) const { return (*((uint32_t*)_address)) == (*((uint32_t*)addr._address)); };
^
IPAddress.h:47:114: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
bool operator==(const IPAddress& addr) const { return (*((uint32_t*)_address)) == (*((uint32_t*)addr._address)); };
Converting between unrelated types like this is commonly done using a union,
which do not break the strict-aliasing rules. Using that union, inside
IPAddress there is now an attribute _address.bytes for the raw byte
arra, or _address.dword for the uint32_t version.
Since we now have easy access to the uint32_t version, this also removes
two memcpy invocations that can just become assignments.
This patch does not change the generated code in any way, the compiler
already optimized away the memcpy calls and the previous casts mean
exactly the same.
This is a different implementation of a part of arduino#1399 and it helps
toward fixing arduino#1728.
ollie1400
pushed a commit
to ollie1400/Arduino
that referenced
this pull request
May 2, 2022
peekNextDigit() returns an int, so it can return -1 in addition to all 256 possible bytes. By putting the result in a signe char, all bytes over 128 will be interpreted as "no bytes available". Furthermore, it seems that on SAM "char" is unsigned by default, causing the "if (c < 0)" line a bit further down to always be false. Using an int is more appropriate. A different fix for this issue was suggested in arduino#1399. This fix helps towards arduino#1728.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Cleaned up warnings about unsigned types, unused variables, and
declarations.
Explicitly declare char as signed for cases when compiling with
-funsigned-char.
Fixed strict-aliasing issues in IPAddress.h.
Fixed array overrun where EXTERNAL_NUM_INTERRUPTS was defined too small.